Refactor/#79 코드 가독성을 위한 리팩토링 #80
Conversation
- 조금 더 일반적인 이름
- 스켈레톤 UI 컴포넌트 구현 - PlaceListItem.Skeleton 형태로 서브 컴포넌트 패턴 적용
- 개별 컴포넌트(MostLikes, MostViews)를 제거하고 RankingPlaceList로 통합 - 정렬 기준(SortType)을 Prop으로 주입받도록 구조 개선하여 재사용성 증대 - UI 레이아웃과 데이터 페칭 로직의 관심사 분리
- URL에서 카테고리 ID를 읽고 업데이트하는 로직을 useCategoryIdFromUrl 훅으로 분리 - CategoryDetailPage의 라우트 업데이트 및 상태 관리 로직 간소화 - 컴포넌트 책임 분리 및 재사용성 향상
- 카테고리 상세 페이지에서 좌우 드래그 제스처로 이전/다음 카테고리 이동 가능하도록 구현 - swipe 방향 및 강도 계산 로직을 별도 컴포넌트(SwipeableArea)로 추상화 - 상태 관리(setActiveCategoryId)와 UI 영역(children) 분리로 구조 개선
WalkthroughPlaceListItem 컴포넌트에 Skeleton 정적 속성을 추가하고, 두 개의 랭킹 컴포넌트(MostLikesPlaces, MostViewsPlaces)를 제거한 후 제너릭 RankingPlaceList로 통합했습니다. 카테고리 페이지에 SwipeableArea 컴포넌트를 추가하고 URL 기반 카테고리 ID 관리를 위한 훅을 도입했습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 세심한 검토가 필요한 영역:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/web/app/categories/[id]/_components/SwipeableArea/SwipeableArea.tsx (1)
1-52: Swipe 범위 상수와 타입 의존성 부분에서 약간의 유연성 개선 여지가 있습니다
현재
1~15범위가SWIPE_CONFIDENCE_THRESHOLD아래 로직과dragConstraints에 하드코딩돼 있어서, 카테고리 개수가 늘어나거나 줄어들면 이 파일을 직접 수정해야 합니다.
상위에서maxCategoryId(또는categories.length)를 내려받아 사용하는 식으로 범위를 주입하면 유지보수가 훨씬 수월해질 것 같습니다.TS를 사용 중이고 전역
React네임스페이스를 따로 선언하지 않았다면,children: React.ReactNode타입을 쓰려면 다음처럼 명시적으로 import 하는 쪽이 안전합니다.import type { ReactNode } from 'react' type Props = { categoryId: string setCategoryId: (id: string) => void children: ReactNode }apps/web/app/categories/[id]/_components/Places/Places.tsx (1)
8-35: Places의 책임 축소 및 categoryId 기반 쿼리 사용이 명확합니다
Props를categoryId하나만 받도록 정리하고,usePlaceQueries.byCategory(categoryId, campus)만 책임지도록 한 구조가 깔끔합니다.- 빈 결과일 때
EmptyPlaces, 그 외에는VerticalScrollArea안에PlaceListItem리스트를 렌더링하는 분기 역시 역할이 명확합니다.showCategory={false}, 마지막 아이템만showBorder=false로 두는 처리도 UI 요구사항에 잘 맞는 것 같습니다.굳이 변경하지 않아도 되지만, JSX는 Fragment 없이 삼항식만 바로 반환해도 되므로 취향에 따라 단순화할 수는 있겠습니다.
apps/web/app/_components/PlaceListItem/PlaceListItem.tsx (1)
1-58: TypeScript static property typing may need explicit interface definitionThe static assignment
PlaceListItem.Skeleton = PlaceListItemSkeletoncould trigger a TypeScript warning about theSkeletonproperty not being declared. Consider augmenting the component type with an intersection to explicitly define theSkeletonsignature:- export const PlaceListItem = ({ + type PlaceListItemComponent = typeof PlaceListItem & { Skeleton: typeof PlaceListItemSkeleton } + + export const PlaceListItem = ({Then apply the type cast at the assignment, or use a separate export pattern.
Regarding the
'use client'directive: PlaceListItem is imported exclusively by client components (CategoryDetailPage, RankingPlaceList, RequestPlacesList), so the current pattern works without violations. Adding'use client'is optional based on your project conventions.apps/web/app/categories/[id]/CategoryDetailPage.tsx (1)
41-50: SwipeableArea의 카테고리 범위(1~15)가 하드코딩되어 있어 카테고리 변경에 취약합니다.현재 SwipeableArea 구현은
1과15를 경계로 사용하고 있고, 여기서는categoryId만 내려주고 있어 실제 카테고리 개수/ID 스키마와 분리되어 있습니다. 카테고리 수가 늘거나 ID가 바뀌면 Swipe 제스처 범위와 URL 상태가 쉽게 어긋날 수 있습니다.
categories길이나 최대 ID를 기반으로 상한선을 계산해 SwipeableArea에 prop으로 내려주거나, SwipeableArea 쪽에서 공용 설정/쿼리를 참조해 매직 넘버를 제거하는 리팩터를 한 번 고려해 보시면 좋겠습니다. (기능상 당장 문제는 아니고, 구조 개선 관점의 제안입니다.)apps/web/app/_components/RankingPlaceList/RankingPlaceList.tsx (1)
30-51: 랭킹 결과가 빈 배열일 때의 UX를 한 번 검토해 보시면 좋겠습니다.현재
PlaceList는places를 바로map해서<ul>안에 렌더링하고 있어, 랭킹 결과가 비어 있으면 아무 항목도 없는 리스트만 표시됩니다. 랭킹 API 특성상 데이터가 항상 있다고 보셔도 되지만, 예외 상황(신규 캠퍼스, 필터 조건 등)을 고려하면 간단한 “데이터 없음”/“랭킹 준비 중” 같은 Empty 상태 컴포넌트를 추가하는 것도 선택지일 것 같습니다. (필수는 아니고 UX 측면의 제안입니다.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/web/app/_components/PlaceListItem/PlaceListItem.tsx(2 hunks)apps/web/app/_components/PlaceListItem/PlaceListItemSkeleton.tsx(1 hunks)apps/web/app/_components/RankingPlaceList/MostLikesPlaces/MostLikesPlaces.tsx(0 hunks)apps/web/app/_components/RankingPlaceList/MostLikesPlaces/index.tsx(0 hunks)apps/web/app/_components/RankingPlaceList/MostViewsPlaces/MostViewsPlaces.tsx(0 hunks)apps/web/app/_components/RankingPlaceList/MostViewsPlaces/index.tsx(0 hunks)apps/web/app/_components/RankingPlaceList/RankingPlaceList.tsx(1 hunks)apps/web/app/_components/RankingPlaceList/index.tsx(0 hunks)apps/web/app/categories/[id]/CategoryDetailPage.tsx(2 hunks)apps/web/app/categories/[id]/_components/Places/Places.tsx(1 hunks)apps/web/app/categories/[id]/_components/RowCategories/RowCategories.tsx(1 hunks)apps/web/app/categories/[id]/_components/SwipeableArea/SwipeableArea.tsx(1 hunks)apps/web/app/categories/[id]/_components/SwipeableArea/index.ts(1 hunks)apps/web/app/categories/[id]/_hooks/useCategoryIdFromUrl.ts(1 hunks)apps/web/app/page.tsx(3 hunks)
💤 Files with no reviewable changes (5)
- apps/web/app/_components/RankingPlaceList/index.tsx
- apps/web/app/_components/RankingPlaceList/MostLikesPlaces/MostLikesPlaces.tsx
- apps/web/app/_components/RankingPlaceList/MostLikesPlaces/index.tsx
- apps/web/app/_components/RankingPlaceList/MostViewsPlaces/index.tsx
- apps/web/app/_components/RankingPlaceList/MostViewsPlaces/MostViewsPlaces.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
apps/web/app/categories/[id]/_components/RowCategories/RowCategories.tsx (3)
apps/web/app/categories/[id]/_components/RowCategories/index.tsx (1)
RowCategories(1-1)apps/web/app/_mocks/data/category.ts (1)
category(1-17)apps/web/app/categories/[id]/_components/RowCategories/CategoryItem.tsx (1)
CategoryItem(14-49)
apps/web/app/_components/PlaceListItem/PlaceListItem.tsx (3)
apps/web/app/requests/_components/PlaceListItem/PlaceListItem.tsx (1)
PlaceListItem(10-42)apps/web/app/_components/PlaceListItem/index.tsx (1)
PlaceListItem(1-1)apps/web/app/_components/PlaceListItem/PlaceListItemSkeleton.tsx (1)
PlaceListItemSkeleton(11-29)
apps/web/app/_components/RankingPlaceList/RankingPlaceList.tsx (3)
apps/web/app/_apis/schemas/place.ts (1)
RankingPlaceSort(74-74)apps/web/app/_store/campus.ts (1)
useCampusStore(9-12)apps/web/app/_apis/queries/place.ts (1)
usePlaceQueries(27-69)
apps/web/app/categories/[id]/_components/Places/Places.tsx (5)
apps/web/app/_store/campus.ts (1)
useCampusStore(9-12)apps/web/app/_apis/queries/place.ts (1)
usePlaceQueries(27-69)apps/web/app/categories/[id]/_components/Places/EmptyPlaces.tsx (1)
EmptyPlaces(9-103)packages/ui/src/components/Layout/VerticalScrollArea/VerticalScrollArea.tsx (1)
VerticalScrollArea(4-20)apps/web/app/_components/PlaceListItem/PlaceListItem.tsx (1)
PlaceListItem(16-56)
apps/web/app/_components/PlaceListItem/PlaceListItemSkeleton.tsx (1)
packages/ui/src/utils/cn.ts (1)
cn(19-21)
apps/web/app/categories/[id]/CategoryDetailPage.tsx (5)
apps/web/app/_apis/queries/category.ts (1)
useCategoryQueries(11-17)apps/web/app/categories/[id]/_hooks/useCategoryIdFromUrl.ts (1)
useCategoryIdFromUrl(4-11)apps/web/app/_mocks/data/category.ts (1)
category(1-17)apps/web/app/categories/[id]/_components/SwipeableArea/SwipeableArea.tsx (1)
SwipeableArea(11-52)apps/web/app/categories/[id]/_components/Places/Places.tsx (1)
Places(12-36)
🔇 Additional comments (5)
apps/web/app/categories/[id]/_components/SwipeableArea/index.ts (1)
1-1: 디렉터리 엔트리 포인트 re-export 패턴 적절함
SwipeableArea를 인덱스에서 재노출하는 패턴이라 import 경로가 단순해지고, 구조상 문제 없어 보입니다.apps/web/app/_components/PlaceListItem/PlaceListItemSkeleton.tsx (1)
1-29: PlaceListItemSkeleton 구조 및 사용 의도가 깔끔하게 잘 잡혀 있습니다
- 기본
count = 3으로 세로 리스트 스켈레톤을 렌더링하는 구조가 실제PlaceListItem과 잘 대응되고,Array.from({ length: count })+ index key도 단순 렌더링 스켈레톤 용도로는 충분합니다.- 공통 레이아웃(
Column,Flex,cn)을 재사용해서 스타일 일관성도 유지되고 있습니다.현재 형태 그대로 사용하셔도 무방해 보입니다.
apps/web/app/page.tsx (1)
9-65: RankingPlaceList 통합과 섹션별 하이드레이션 구조가 일관성 있게 정리되었습니다
MostLikesPlaces/MostViewsPlaces를 제거하고RankingPlaceList하나를 서로 다른 props로 두 번 사용하는 패턴이 중복을 잘 줄여 줍니다.- 카테고리 영역만
HydrationBoundaryPage로 감싸useCategoryQueries.list()만 사전 prefetch 하는 구조도 데이터/레이아웃 구분 의도에 잘 맞습니다.- 나머지 배너, 랭킹 섹션, 네비게이션은 기존 흐름을 유지하고 있어 동작 면에서도 큰 리스크는 없어 보입니다.
현재 변경 범위 내에서는 그대로 머지해도 무방해 보입니다.
apps/web/app/categories/[id]/_components/RowCategories/RowCategories.tsx (1)
6-34: No changes needed — type consistency is correctThe
Category.idtype is alreadystringdue to Zod's.transform(String)in the schema definition (CategorySchema), so the comparisons and function calls in RowCategories are type-safe and correct as-is. The mock data contains numeric IDs only because it represents raw data before schema validation; the actualCategorytype inferred from the schema hasid: string. The component properly usescategoryId === category.idandsetCategoryId(category.id)without requiring explicit conversions.apps/web/app/_components/RankingPlaceList/RankingPlaceList.tsx (1)
19-27: 랭킹 리스트 레이아웃과 데이터 패칭 분리가 명확해서 유지보수에 좋아 보입니다.
RankingPlaceList가 제목/아이콘/정렬 기준만 받고, 실제 데이터 패칭과 리스트 렌더링을 내부PlaceList+Suspense경계에 위임한 구조가 깔끔합니다. 상위에서는 섹션 UI만 신경 쓰면 되어서 다른 랭킹 섹션 추가/변경 시에도 재사용성이 높을 것 같습니다.
| import { usePathname } from 'next/navigation' | ||
| import { CLIENT_PATH } from '@/_constants/path' | ||
|
|
||
| export const useCategoryIdFromUrl = () => { | ||
| const categoryId = usePathname().split('/')[2] || '0' | ||
| const setCategoryId = (id: string) => { | ||
| window.history.replaceState(null, '', CLIENT_PATH.CATEGORY_DETAIL(id)) | ||
| } | ||
|
|
||
| return [categoryId, setCategoryId] as const | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find usages of useCategoryIdFromUrl hook
rg "useCategoryIdFromUrl" --type ts --type tsx -B 2 -A 2Repository: knu-matzip/Matzip-Client
Length of output: 95
🏁 Script executed:
# Find SwipeableArea and RowCategories components mentioned in the review
fd "SwipeableArea|RowCategories" --type f -e ts -e tsxRepository: knu-matzip/Matzip-Client
Length of output: 213
🏁 Script executed:
# Check the actual file content and its context
cat -n "apps/web/app/categories/[id]/_hooks/useCategoryIdFromUrl.ts"Repository: knu-matzip/Matzip-Client
Length of output: 509
🏁 Script executed:
# Search for setCategoryId usage specifically
rg "setCategoryId" --type ts --type tsx -B 3 -A 3Repository: knu-matzip/Matzip-Client
Length of output: 95
🏁 Script executed:
# Correct search for useCategoryIdFromUrl usage with proper file types
rg "useCategoryIdFromUrl" --type ts --type js -B 2 -A 5Repository: knu-matzip/Matzip-Client
Length of output: 2334
🏁 Script executed:
# Read RowCategories component
cat -n "apps/web/app/categories/[id]/_components/RowCategories/RowCategories.tsx"Repository: knu-matzip/Matzip-Client
Length of output: 1487
🏁 Script executed:
# Read SwipeableArea component
cat -n "apps/web/app/categories/[id]/_components/SwipeableArea/SwipeableArea.tsx"Repository: knu-matzip/Matzip-Client
Length of output: 1713
🏁 Script executed:
# Read full CategoryDetailPage.tsx to see the useEffect
cat -n "apps/web/app/categories/[id]/CategoryDetailPage.tsx"Repository: knu-matzip/Matzip-Client
Length of output: 2258
🏁 Script executed:
# Search for router.replace usage patterns in the codebase
rg "router\.replace" --type ts --type js -B 2 -A 2Repository: knu-matzip/Matzip-Client
Length of output: 50
🏁 Script executed:
# Check if there's any useEffect with pathname dependency in CategoryDetailPage or similar files
rg "useEffect.*pathname|pathname.*useEffect" --type ts --type js -B 3 -A 5Repository: knu-matzip/Matzip-Client
Length of output: 50
🏁 Script executed:
# Double-check: search for useRouter import to see if router is used elsewhere in the app
rg "useRouter" --type ts --type js -l | head -20Repository: knu-matzip/Matzip-Client
Length of output: 389
🏁 Script executed:
# Verify CLIENT_PATH.CATEGORY_DETAIL function to understand the path format
rg "CATEGORY_DETAIL" --type ts --type js -B 2 -A 2Repository: knu-matzip/Matzip-Client
Length of output: 1800
Use router.replace() instead of window.history.replaceState to ensure usePathname updates
The hook computes categoryId from usePathname() once at render time, but setCategoryId only calls window.history.replaceState, which doesn't trigger Next.js router navigation. This means:
- URL updates in the browser but
usePathname()doesn't re-evaluate categoryIdremains staleSwipeableArea'skey={categoryId}doesn't update, so the motion animation doesn't re-mount- Components receive outdated
categoryIdvalues
When SwipeableArea or RowCategories call setCategoryId, the UI won't reflect the change. Using router.replace() instead triggers proper router navigation, making usePathname() re-run and the component re-render with the correct category ID.
-import { usePathname } from 'next/navigation'
+import { usePathname, useRouter } from 'next/navigation'
import { CLIENT_PATH } from '@/_constants/path'
export const useCategoryIdFromUrl = () => {
+ const router = useRouter()
const categoryId = usePathname().split('/')[2] || '0'
const setCategoryId = (id: string) => {
- window.history.replaceState(null, '', CLIENT_PATH.CATEGORY_DETAIL(id))
+ router.replace(CLIENT_PATH.CATEGORY_DETAIL(id))
}
return [categoryId, setCategoryId] as const
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { usePathname } from 'next/navigation' | |
| import { CLIENT_PATH } from '@/_constants/path' | |
| export const useCategoryIdFromUrl = () => { | |
| const categoryId = usePathname().split('/')[2] || '0' | |
| const setCategoryId = (id: string) => { | |
| window.history.replaceState(null, '', CLIENT_PATH.CATEGORY_DETAIL(id)) | |
| } | |
| return [categoryId, setCategoryId] as const | |
| } | |
| import { usePathname, useRouter } from 'next/navigation' | |
| import { CLIENT_PATH } from '@/_constants/path' | |
| export const useCategoryIdFromUrl = () => { | |
| const router = useRouter() | |
| const categoryId = usePathname().split('/')[2] || '0' | |
| const setCategoryId = (id: string) => { | |
| router.replace(CLIENT_PATH.CATEGORY_DETAIL(id)) | |
| } | |
| return [categoryId, setCategoryId] as const | |
| } |
🤖 Prompt for AI Agents
In apps/web/app/categories/[id]/_hooks/useCategoryIdFromUrl.ts lines 1-11, the
hook currently uses window.history.replaceState which does not trigger Next.js
navigation so usePathname() never updates; replace that call by importing and
using the app-router's replace API (import { useRouter } from
'next/navigation'), call router.replace(CLIENT_PATH.CATEGORY_DETAIL(id)) inside
setCategoryId, and get router via const router = useRouter(); keep returning
[categoryId, setCategoryId] as const so usePathname re-evaluates and components
remount correctly.
| const [categoryId, setCategoryId] = useCategoryIdFromUrl() | ||
| const activeCategory = categories.find( | ||
| (category) => category.id === activeCategoryId, | ||
| (category) => category.id === categoryId, | ||
| ) |
There was a problem hiding this comment.
activeCategory 계산 시 숫자/문자열 비교로 매칭이 항상 실패합니다.
useCategoryIdFromUrl가 문자열을 반환하는 반면, 카테고리 데이터의 id는 mock 기준 number입니다. 현재처럼 category.id === categoryId로 strict 비교하면 어떤 카테고리도 매칭되지 않아 activeCategory가 항상 undefined가 됩니다(문서 제목과 헤더 아이콘/이름 모두 깨짐).
문자열로 맞춰 비교하도록 수정하는 쪽이 안전해 보입니다.
- const activeCategory = categories.find(
- (category) => category.id === categoryId,
- )
+ const activeCategory = categories.find(
+ (category) => String(category.id) === categoryId,
+ )🤖 Prompt for AI Agents
In apps/web/app/categories/[id]/CategoryDetailPage.tsx around lines 18 to 21,
activeCategory is never matched because useCategoryIdFromUrl() returns a string
while category.id in mocks is a number; change the comparison to compare string
forms (e.g., convert category.id to String() before ===) or coerce categoryId to
a number before comparison so the types match; update the equality to use the
same type (string-to-string or number-to-number) and add a small comment
explaining the conversion.
#️⃣연관된 이슈
📝작업 내용
토스 Frontend Fundamentals 1회 모의고사 후기
프로젝트에 적용하여 리팩토링을 진행했습니다.
1. PlaceListItem 스켈레톤 UI 구현 (Compound Component 패턴)
변경 내용:
PlaceListItem.Skeleton 형태로 스켈레톤 컴포넌트를 할당했습니다.
효과:
컴포넌트와 그 로딩 상태(Skeleton) 간의 강한 연관성(Dependency) 을 코드 구조로 명시했습니다.
Import 한 번으로 컴포넌트와 스켈레톤을 함께 사용할 수 있어 응집도를 높였습니다.
2. 랭킹 리스트 컴포넌트 통합 및 추상화 (RankingPlaceList_찜많은 맛집, 오늘의 맛집)
[기존 문제점]
[개선 내용]
RankingPlaceList는 레이아웃과 Suspense 처리를 담당하고, 내부의 PlaceList는 데이터 페칭만 담당하도록 역할을 분리했습니다.
3. 카테고리 URL 상태 관리 훅 (useCategoryIdFromUrl)
[기존 문제점]
[개선 내용]
4. 스와이프 인터랙션 분리 (SwipeableArea)
[기존 문제점]
데이터 페칭 컴포넌트 내부에 복잡한 제스처 계산 로직(pan, velocity 등) 및 상태변환 로직 등 이 포함되어 있었습니다.
[개선 내용]
스크린샷 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선 사항
✏️ Tip: You can customize this high-level summary in your review settings.